Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[connectors] Add a CLI command confluence upsert-page #9358

Merged
merged 18 commits into from
Dec 13, 2024

Conversation

aubin-tchoi
Copy link
Contributor

Description

  • This PR adds two CLI commands: confluence upsert-page and confluence upsert-pages that upsert pages given their IDs.
  • The upsertion correctly fills the parents.

Risk

  • Low, since we control the input the can pinpoint very precisely the data affected.

Deploy Plan

  • Deploy connectors.

@aubin-tchoi aubin-tchoi requested a review from spolu December 13, 2024 12:27
> => {
const logger = topLogger.child({ majorCommand: "confluence", command, args });
switch (command) {
case "upsert-page": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bit of a risk of having some divergence between the cli code and the activities code, we generally try to call an activity from here even if that means it's async.

All that being said I'm fine with having a direct implementation. Any chance we can dry a bit these 2 commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep I agree, I was contemplating adding a new activity but in the end we won't do that in the workflow (upsert and check parents right after)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be an option on the activity + a new workflow to do just one page with that option set

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm interesting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, doing that 👍

Copy link
Contributor Author

@aubin-tchoi aubin-tchoi Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually it's a bit too convoluted to use the same activity: one fetches the page + upserts without parent and is optimized for the workflow (it does some version checking), the other one fetches the page, fetches the space using the page data, resolves parents and does an upsert with parents.

I added a separate activity to do the upsertWithFullParents and added workflows since we might want to do these with retries and history
SGTY @spolu ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yess!

@aubin-tchoi aubin-tchoi force-pushed the confluence-force-upsert branch from 88d86df to 2be5a29 Compare December 13, 2024 18:58
@aubin-tchoi aubin-tchoi force-pushed the confluence-force-upsert branch from 1131c8b to 19716ba Compare December 13, 2024 21:27
@aubin-tchoi aubin-tchoi merged commit 2cf24b7 into main Dec 13, 2024
4 checks passed
@aubin-tchoi aubin-tchoi deleted the confluence-force-upsert branch December 13, 2024 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants